test(swift-sdk): first swift sdk integration tests with local network#3712
test(swift-sdk): first swift sdk integration tests with local network#3712ZocoLini wants to merge 1 commit into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughMigrates Swift SDK testing from many mock/unit tests to a devnet-backed integration test harness: deletes old test suites and headers, adds integration helpers and tests, updates package manifest and build scripts, and changes CI workflow to invoke the new runners. ChangesSwift SDK Integration Test Migration
Sequence DiagramsequenceDiagram
participant Developer as Dev / CI
participant CI as GitHub Actions
participant Runner as run_tests.sh
participant IntegrationRunner as run_integration_tests.sh
participant Dashmate as dashmate (local devnet)
participant Core as dashd (Core RPC)
participant SwiftTests as SwiftDashSDKIntegrationTests
CI->>Runner: run unit build & tests
Runner->>SwiftTests: swift build/test, xcodebuild test
CI->>IntegrationRunner: run integration flow
IntegrationRunner->>Dashmate: reset/start devnet
IntegrationRunner->>Core: Core RPC calls via CoreRPCClient (getnewaddress, sendtoaddress, generatetoaddress)
IntegrationRunner->>SwiftTests: RUN_INTEGRATION_TESTS=1 swift test SwiftDashSDKIntegrationTests
IntegrationRunner->>Dashmate: stop (always)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
b16e06f to
361114b
Compare
361114b to
59ef0d4
Compare
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "b12b8793f11e97a6829590cab1d11aa9e71a103876c4b44407b2d45c93ccdbb1"
)Xcode manual integration:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/swift-sdk/SwiftTests/SwiftDashSDKTests/PlatformWalletTypesTests.swift (1)
27-34: ⚡ Quick winMake odd-length hex behavior explicit and test exact expected output.
This test currently validates an implementation-dependent outcome with only
XCTAssertNotNil, which can hide parsing regressions. Please assert a deterministic contract for odd-length input (reject withnil, or accept with a specific byte result) and verify that exact result.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/swift-sdk/SwiftTests/SwiftDashSDKTests/PlatformWalletTypesTests.swift` around lines 27 - 34, The test testDataFromOddLengthHexString currently only asserts non-nil for an implementation-dependent outcome; change it to assert a deterministic contract: decide whether Data(hexString:) should reject odd-length hex (preferred) or normalize it, then update the test to assert that exact behavior—for example, if you choose rejection, replace XCTAssertNotNil(data) with XCTAssertNil(data) and optionally assert a specific error/path; if you choose acceptance, assert the exact expected bytes (e.g., XCTAssertEqual(data, expectedData)). Ensure the assertion references the Data(hexString:) initializer and the test method name testDataFromOddLengthHexString so the behavior is explicit and reproducible.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/swift-sdk/run_tests.sh`:
- Around line 11-17: The script run_tests.sh currently sets SIM_NAME by parsing
device display names which can be ambiguous; instead derive a SIM_UDID and use
-destination "id=$SIM_UDID". Update run_tests.sh to introduce a SIM_UDID
variable (e.g., SIM_UDID="${SIM_UDID:-}") and, when empty, run xcrun simctl list
devices available to pick the first matching iPhone device UDID (not the display
name). Replace uses of SIM_NAME/-destination "platform=iOS
Simulator,name=$SIM_NAME" with -destination "id=$SIM_UDID" so xcodebuild targets
the exact simulator UDID.
In
`@packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/CoreRPCClient.swift`:
- Around line 74-76: CoreRPCClient.call builds a URLRequest but doesn't set a
timeout, leaving the default (~60s idle) which can hang tests; set an explicit
timeoutInterval on the URLRequest (e.g., request.timeoutInterval = 30) before
using URLSession to bound RPC call time, and ensure any related helper paths in
CoreRPCClient.call respect this timeout when creating the request.
---
Nitpick comments:
In
`@packages/swift-sdk/SwiftTests/SwiftDashSDKTests/PlatformWalletTypesTests.swift`:
- Around line 27-34: The test testDataFromOddLengthHexString currently only
asserts non-nil for an implementation-dependent outcome; change it to assert a
deterministic contract: decide whether Data(hexString:) should reject odd-length
hex (preferred) or normalize it, then update the test to assert that exact
behavior—for example, if you choose rejection, replace XCTAssertNotNil(data)
with XCTAssertNil(data) and optionally assert a specific error/path; if you
choose acceptance, assert the exact expected bytes (e.g., XCTAssertEqual(data,
expectedData)). Ensure the assertion references the Data(hexString:) initializer
and the test method name testDataFromOddLengthHexString so the behavior is
explicit and reproducible.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0a0c245b-b203-4fe5-8d83-2191641ba8ff
📒 Files selected for processing (40)
.github/workflows/swift-sdk-build.yml.serena/project.ymlpackages/swift-sdk/Package.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/README.mdpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swiftpackages/swift-sdk/SwiftTests/Package.swiftpackages/swift-sdk/SwiftTests/Sources/SwiftDashSDKMock/SwiftDashSDK.hpackages/swift-sdk/SwiftTests/Sources/SwiftDashSDKMock/SwiftDashSDKMock.cpackages/swift-sdk/SwiftTests/SwiftDashSDK.hpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/CoreSendIntegrationTests.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Platform/SDKBootstrapIntegrationTests.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/CoreRPCClient.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/IntegrationTestCase.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/IntegrationTestEnv.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/LocalDevnet.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/Shell.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/TestWallet.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/IdentityManagerTests.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/PlatformWalletIntegrationTests.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/PlatformWalletTests.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/PlatformWalletTypesTests.swiftpackages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/ContactRequestTests.swiftpackages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/DataContractTests.swiftpackages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/DocumentTests.swiftpackages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/EstablishedContactTests.swiftpackages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/IdentityManagerTests.swiftpackages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/IdentityTests.swiftpackages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/ManagedIdentityTests.swiftpackages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/MemoryManagementTests.swiftpackages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/PlatformWalletIntegrationTests.swiftpackages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/PlatformWalletTypesTests.swiftpackages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/SDKTests.swiftpackages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/SwiftConstants.swiftpackages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/TokenTests.swiftpackages/swift-sdk/SwiftTests/run_tests.shpackages/swift-sdk/Tests/SwiftDashSDKTests/KeyWallet/WalletSerializationTests.swiftpackages/swift-sdk/build_ios.shpackages/swift-sdk/example/SwiftSDKExample.swiftpackages/swift-sdk/run_integration_tests.shpackages/swift-sdk/run_tests.sh
💤 Files with no reviewable changes (21)
- packages/swift-sdk/Tests/SwiftDashSDKTests/KeyWallet/WalletSerializationTests.swift
- packages/swift-sdk/SwiftTests/Sources/SwiftDashSDKMock/SwiftDashSDK.h
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/SwiftConstants.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/PlatformWalletIntegrationTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/DocumentTests.swift
- packages/swift-sdk/example/SwiftSDKExample.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/MemoryManagementTests.swift
- packages/swift-sdk/SwiftTests/Package.swift
- packages/swift-sdk/SwiftTests/SwiftDashSDK.h
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/EstablishedContactTests.swift
- packages/swift-sdk/SwiftTests/run_tests.sh
- packages/swift-sdk/SwiftTests/Sources/SwiftDashSDKMock/SwiftDashSDKMock.c
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/PlatformWalletTypesTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/ManagedIdentityTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/IdentityTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/IdentityManagerTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/ContactRequestTests.swift
- packages/swift-sdk/SwiftTests/SwiftDashSDKTests/PlatformWalletTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/TokenTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/SDKTests.swift
- packages/swift-sdk/SwiftTests/Tests/SwiftDashSDKTests/DataContractTests.swift
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The PR adds a Swift SDK integration-test framework against a local dashmate devnet. Validation confirmed: the shared SwiftData container makes the global transaction-count assertion brittle, the missing README pointer is real, the DASHMATE_CONFIG env var is read by the test binary but ignored by the lifecycle/probe in the runner, and ensureSPVStarted/trackedWalletIds have unsynchronized check-then-act on nonisolated(unsafe) state. No in-scope blocker; recommending COMMENT.
🟡 3 suggestion(s) | 💬 3 nitpick(s)
Out-of-scope follow-up suggestions (2)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Net loss of Swift unit-test coverage from mock-based suite removal — The PR deletes ~2500 lines of older mock-based unit tests (Document/Identity/MemoryManagement/Token/ContactRequest/DataContract/EstablishedContact/ManagedIdentity/SDK/WalletSerialization) and replaces them with ~140 lines of trivial getter/error-path coverage. The deletions are justified — the mocks didn't reflect the real FFI — but the replacement is much thinner. Out of scope because recreating that coverage against real FFI is a substantial separate effort.
- Follow-up: Open a follow-up issue enumerating behaviors the deleted unit tests covered (document CRUD, opaque-handle memory management, token ops) and decide which deserve real-FFI unit or integration coverage.
- Integration suite is skipped in CI by design — can rot silently —
run_tests.shdoes not setRUN_INTEGRATION_TESTS=1, so the new integration tests all hitXCTSkipUnlessin CI. This is the intended split (CI runs unit + xcodebuild example app; dashmate-dependent suite runs locally), but it means the framework itself is never exercised in CI and regressions can land unnoticed.- Follow-up: Track adding a self-hosted/nightly job that provisions dashmate and runs
run_integration_tests.sh.
- Follow-up: Track adding a self-hosted/nightly job that provisions dashmate and runs
59ef0d4 to
ea8c626
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/swift-sdk-build.yml:
- Around line 95-98: The workflow currently disables failure handling with "set
+e" and forces success with a trailing "true", causing failures in "yarn
dashmate group stop --group=local --force" and "yarn dashmate group reset
--group=local --hard --verbose" to be ignored; restore strict error handling by
removing "set +e" and the final "true" (or re-enable with "set -e" after the
block) so any non-zero exit from those yarn dashmate commands fails the job, or
explicitly propagate errors by appending "|| exit 1" to the reset command.
In
`@packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/IntegrationTestCase.swift`:
- Line 9: The shared cached variable bootstrapResult in IntegrationTestCase is
accessed from nonisolated(unsafe) context in sharedEnv() and can race; protect
all reads/writes to bootstrapResult by introducing synchronization (e.g., a
private static serial DispatchQueue or an actor) and marshal all accesses
through it inside sharedEnv() and any other accessors; update sharedEnv() to
perform the check/read/write of bootstrapResult on that queue (or via await if
using an actor) so initialization is atomic and thread-safe while keeping the
existing API.
In
`@packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/IntegrationTestEnv.swift`:
- Around line 18-20: IntegrationTestEnv currently exposes nonisolated(unsafe)
shared mutable state via spvStarted and trackedWalletIds and performs an
unsynchronized check/set in ensureSPVStarted(), causing race conditions; fix by
moving these vars into a synchronized context (either make IntegrationTestEnv an
actor or add a private serial synchronization primitive such as a dedicated
DispatchQueue or Lock) and update all accessors (ensureSPVStarted(), any
append/iterate/clear operations on trackedWalletIds) to run on that
synchronization context so the check-and-set in ensureSPVStarted() is atomic (no
await between check and set) and all mutations/read of trackedWalletIds are
serialized.
In
`@packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/Shell.swift`:
- Around line 74-77: The code currently calls process.waitUntilExit() before
reading stdout/stderr which can deadlock; change Shell.run (or the function
creating Process and using stdoutPipe/stderrPipe) to read the pipes
asynchronously: set stdoutPipe.fileHandleForReading.readabilityHandler and
stderrPipe.fileHandleForReading.readabilityHandler to append incoming Data into
local Data buffers, then call process.waitUntilExit(), then clear the
readabilityHandlers and use the accumulated Data (instead of calling
readDataToEndOfFile() after waitUntilExit()). Ensure handlers are removed (set
to nil) to avoid leaks and convert the collected Data to strings as before.
In
`@packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/TestWallet.swift`:
- Around line 44-49: The loop currently checks the deadline before calling
condition(), which can suppress an exception thrown by condition() on the final
attempt; change the loop in the waiting helper (the block that creates deadline,
calls try await condition(), sleeps with Task.sleep using pollInterval, and
throws TimeoutError) so you call try await condition() first, return on success,
then if Date() >= deadline throw the TimeoutError, and only then await
Task.sleep; this ensures any error from condition() on the final attempt is
propagated instead of being replaced by the TimeoutError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 44a6106d-d8be-4e58-af49-1a307003d13a
📒 Files selected for processing (11)
.github/workflows/swift-sdk-build.ymlpackages/swift-sdk/Package.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/CoreSendIntegrationTests.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Platform/SDKBootstrapIntegrationTests.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/CoreRPCClient.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/IntegrationTestCase.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/IntegrationTestEnv.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/LocalDevnet.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/Shell.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/TestWallet.swiftpackages/swift-sdk/run_integration_tests.sh
ea8c626 to
def4241
Compare
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta is CI-only: three new workflow steps that reset the local dashmate stack, run the new Swift integration suite via run_integration_tests.sh, and always stop dashmate on exit. No FFI, consensus, crypto, or production paths are touched. All six prior findings against 361114b still apply verbatim at HEAD (none were addressed in the delta), plus the new CI wiring introduces two related concerns around error swallowing on reset and the duplicated --group=local ownership between the workflow and the runner script.
🟡 4 suggestion(s) | 💬 4 nitpick(s)
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Prior
SwiftTestsSwift Package and example app cleanup — The cumulative diff againstv3.1-devdeletespackages/swift-sdk/SwiftTests/Package.swift,SwiftDashSDK.h,run_tests.sh, andexample/SwiftSDKExample.swift. These removals look intentional given the test reorganisation, but they break previously documented entry points that may be referenced from external docs or downstream consumers.- Follow-up: Author-requested follow-up PR: sweep README/docs for references to
SwiftTests/Package.swift,SwiftDashSDK.h, andexample/SwiftSDKExample.swiftand update or repoint them.
- Follow-up: Author-requested follow-up PR: sweep README/docs for references to
9031b50 to
9069b49
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/swift-sdk-build.yml:
- Line 94: The workflow is using floating refs (actions/setup-node@v4 and
cargo-bins/cargo-binstall@v1.3.1); pin both Actions to immutable commit SHAs
instead of version tags to harden supply-chain integrity — locate the
corresponding repository commit SHAs for actions/setup-node and
cargo-bins/cargo-binstall (from their GitHub repos or Marketplace pages) and
replace the usages "actions/setup-node@v4" and
"cargo-bins/cargo-binstall@v1.3.1" with "actions/setup-node@<commit-sha>" and
"cargo-bins/cargo-binstall@<commit-sha>" respectively, keeping the rest of the
workflow unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d295ba4-9bf0-4735-bc72-4c3159f6e942
📒 Files selected for processing (11)
.github/workflows/swift-sdk-build.ymlpackages/swift-sdk/Package.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/CoreSendIntegrationTests.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Platform/SDKBootstrapIntegrationTests.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/CoreRPCClient.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/IntegrationTestCase.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/IntegrationTestEnv.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/LocalDevnet.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/Shell.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/TestWallet.swiftpackages/swift-sdk/run_integration_tests.sh
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The latest delta only adds a Node 22 pinning step and a corepack-enable step to the CI workflow before the dashmate lifecycle. None of the eight previously reported findings were addressed; all are reproducible at def4241 and carried forward. No new issues introduced by the delta itself.
🟡 4 suggestion(s) | 💬 4 nitpick(s)
2 additional finding(s) omitted (not in diff).
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Self-hosted runner global-state side effects from setup-node + corepack — The newly added
actions/setup-node@v4+corepack enablesteps run on a self-hosted macOS ARM64 runner. setup-node mutates PATH and corepack-managed yarn shims for the runner user; on long-lived self-hosted runners this can leave state behind between jobs (e.g. pinning Node 22 for subsequent unrelated workflows that expect system Node). Worth tracking but not blocking this PR.- Follow-up: Create a separate issue to audit self-hosted runner global-state interactions from setup-node and corepack across the platform CI matrix.
There was a problem hiding this comment.
Code Review
Verified all 8 prior findings against current head 9031b50 — all remain STILL VALID and are carried forward. The latest delta (def4241..9031b50) only adds CI provisioning of cargo-binstall, wasm-bindgen-cli@0.2.108, and wasm-pack to .github/workflows/swift-sdk-build.yml. One new finding: wasm-pack is installed unpinned alongside a pinned wasm-bindgen-cli, which weakens CI reproducibility. No blocking issues; all findings are suggestions/nitpicks against new test/CI infrastructure.
🟡 4 suggestion(s) | 💬 5 nitpick(s)
Most findings were posted inline. GitHub could not place the workflow findings on the current diff, so I’m including those carried-forward/latest-delta items here instead of letting them disappear:
Additional verified findings not posted inline
🟡 Suggestion: Carried-forward prior finding: reset step swallows all errors, hiding broken-devnet failures from CI
.github/workflows/swift-sdk-build.yml lines 110-116
The block uses set +e and a trailing true, so the step always exits 0 — even if yarn dashmate group reset --group=local --hard --verbose fails (corrupt state, missing config, permission issue). The integration-tests step then runs against stale or broken devnet state and can produce misleading failures or false greens. The wasm tooling steps added at lines 101–108 do not change this. Fail loudly on reset errors, or scope set +e to only the expected 'nothing to stop' case from group stop.
💬 Nitpick: New finding in latest delta: wasm-pack version is unpinned in CI
.github/workflows/swift-sdk-build.yml lines 107-108
The delta adds cargo binstall wasm-pack -y without a version pin, while the sibling step pins wasm-bindgen-cli@0.2.108. cargo binstall resolves to the latest published wasm-pack each time CI runs, so an upstream release can break the integration-test job on a green PR with no change in this repo. Pin wasm-pack to a known-good version (consistent with the wasm-bindgen-cli pin) so CI builds remain reproducible.
- name: Install wasm-pack
run: cargo binstall wasm-pack@0.13.1 -y
💬 Nitpick: Carried-forward prior finding: CI workflow and runner script duplicate (and can conflict on) dashmate lifecycle
.github/workflows/swift-sdk-build.yml lines 110-125
The workflow resets dashmate (lines 110–116), runs run_integration_tests.sh (118–120), then unconditionally stops dashmate (122–125). The script itself has its own 'start if not reachable' branch (run_integration_tests.sh:55–61) that also calls group stop then group start for --group=local. Two independent lifecycle owners increase the chance of ordering / config drift between local and CI, and make failures harder to diagnose. Consolidate ownership in one layer.
9069b49 to
ef01ba9
Compare
8c51c88 to
e345afd
Compare
e345afd to
707307c
Compare
04e1818 to
d3615e2
Compare
LuuOW
left a comment
There was a problem hiding this comment.
Technical audit: Implementation verified for system consistency and engineering integrity.
c49eaff to
074ee15
Compare
074ee15 to
7fc319d
Compare
Issue being fixed or feature implemented
What was done?
How Has This Been Tested?
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Tests
Chores
Documentation